Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Formats codebase #2531

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Formats codebase #2531

merged 3 commits into from
Nov 6, 2023

Conversation

MaicolAntali
Copy link
Contributor

@MaicolAntali MaicolAntali commented Nov 6, 2023

Purpose

Reformat the Gson codebase to follow the Google Java Style Guide

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

* @see #fromJson(String, Class)
* @see #fromJson(Reader, TypeToken)
*/
public <T> T fromJson(Reader json, Class<T> classOfT) throws JsonSyntaxException, JsonIOException {
public <T> T fromJson(Reader json, Class<T> classOfT)

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method Gson.fromJson(..) could be confused with overloaded method
fromJson
, since dispatch depends on static types.
Comment on lines +847 to +848
List<TypeAdapterFactory> factories =
new ArrayList<>(this.factories.size() + this.hierarchyFactories.size() + 3);

Check notice

Code scanning / CodeQL

Possible confusion of local and field Note

Potentially confusing name: method
create
also refers to field
factories
(as this.factories).
@SuppressWarnings("deprecation") // superclass constructor
public JsonObject() {
}
public JsonObject() {}

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
JsonElement.JsonElement
should be avoided because it has been deprecated.
// if the value has no time component (and no time zone), we are done
boolean hasT = checkOffset(date, offset, 'T');

if (!hasT && (date.length() <= offset)) {

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
date
may be null at this access as suggested by
this
null guard.
} else {
throw new IndexOutOfBoundsException("Invalid time zone indicator '" + timezoneIndicator+"'");
// second and milliseconds can be optional
if (date.length() > offset) {

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
date
may be null at this access as suggested by
this
null guard.
Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@eamonnmcmanus eamonnmcmanus merged commit 2c94c75 into google:main Nov 6, 2023
9 checks passed
@MaicolAntali MaicolAntali deleted the formats-codebase branch November 6, 2023 20:08
@Marcono1234 Marcono1234 mentioned this pull request Nov 6, 2023
9 tasks
@Marcono1234
Copy link
Collaborator

For anyone who wants to verify that these changes are legit, do the following:

  1. Check out commit cb6643f (the commit before the formatting change)

  2. Add the following to the root pom.xml:

    Spotless plugin
    <!-- Spotless plugin: keeps the code formatted following the google-java-styleguide -->
    <plugin>
      <groupId>com.diffplug.spotless</groupId>
      <artifactId>spotless-maven-plugin</artifactId>
      <version>2.40.0</version>
      <executions>
        <execution>
          <goals>
            <goal>check</goal>
          </goals>
        </execution>
      </executions>
      <configuration>
        <formats>
          <format>
            <includes>
              <include>*.md</include>
              <include>.gitignore</include>
            </includes>
            <trimTrailingWhitespace/>
            <endWithNewline/>
            <indent>
              <spaces>true</spaces>
              <spacesPerTab>4</spacesPerTab>
            </indent>
          </format>
        </formats>
        <java>
          <googleJavaFormat>
            <style>GOOGLE</style>
            <reflowLongStrings>true</reflowLongStrings>
            <formatJavadoc>true</formatJavadoc>
          </googleJavaFormat>
          <importOrder />
          <removeUnusedImports />
          <formatAnnotations />     <!-- Puts type annotations immediately before types. -->.
        </java>
      </configuration>
    </plugin>
  3. Run mvn spotless:apply

You should obtain a state nearly identical to what this PR includes. The only changes are in 26 files were comments were modified (I assume because the Spotless plugin / Google Formatter did not properly wrap them).

@MaicolAntali, just for clarification, you edited those comments, right?

@eamonnmcmanus
Copy link
Member

FWIW the check I used was to import this change into Google's build system, which does not include timestamps in jar files. Then compile before and after with -g:none (since some line numbers changed) and verify that the resultant Gson jar file is identical.

@MaicolAntali
Copy link
Contributor Author

MaicolAntali commented Nov 13, 2023

The only changes are in 26 files were comments were modified (I assume because the Spotless plugin / Google Formatter did not properly wrap them).

Exactly,

What I have done:

  • Put comments on new line
  • Fix some long single line (//) comments

Original:

// Lorem Ipsum is simply dummy text of the printing and typesetting industry. 
// Lorem Ipsum has been the industry's standard dummy text ever since the 1500s,
// when an unknown printer took a galley of type and scrambled it to make a type specimen book. 

Edited by spotless:

// Lorem Ipsum is simply dummy text of the printing and 
// typesetting industry. 
// Lorem Ipsum has been the industry's standard dummy text ever since the 
// 1500s,
// when an unknown printer took a galley of type and scrambled it to make a type specimen book. 

Committed:

// Lorem Ipsum is simply dummy text of the printing and 
// typesetting industry. Lorem Ipsum has been the industry's 
// standard dummy text ever since the 1500s, when an 
// unknown printer took a ...

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Nov 13, 2023

Thanks for the confirmation from you both and thanks @MaicolAntali for your dedication with this by manually improving the comment formatting!

FWIW the check I used was to import this change into Google's build system, which does not include timestamps in jar files. Then compile before and after with -g:none (since some line numbers changed) and verify that the resultant Gson jar file is identical.

Thanks for the hint. It looks like locally you can achieve this using the following Maven command:

mvn clean verify jar:test-jar javadoc:jar "-Dproguard.skip" -DskipTests "-Dmaven.compiler.debug=true" "-Dmaven.compiler.debuglevel=none" "-Dproject.build.outputTimestamp=2023-01-01T00:00:00Z"

I only tried this on the changes I obtained locally by running mvn spotless:apply (which as mentioned above slightly differ from the changes of this PR); but unless I messed up my test setup, to me it also looks like the main and test JARs were identical before and after the formatting.

@Marcono1234 Marcono1234 mentioned this pull request Nov 14, 2023
9 tasks
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Sep 14, 2024
* Formats `.java` files

* Formats `.md` files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants